1.8.1 #211
1.8.1 #211
Conversation
WalkthroughBumps project versioning to v1.8.1 across CI/release/packaging/docs; adds a generic FetchEndpoint[T] API fetch path and EndpointResource interface with GetResourceName() methods; refactors per-endpoint callers/tests to return a resource name; converts several commands to return errors instead of exiting; and adds async image loading with a spinner in the card viewer. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Command/Handler
participant Fetch as FetchEndpoint[T]
participant ApiSetup as ApiCallSetup
participant HTTP as http.Client
participant API as External API
Caller->>Fetch: FetchEndpoint(endpoint, resourceName, baseURL, type)
Fetch->>ApiSetup: ApiCallSetup(url, target)
ApiSetup->>HTTP: Do(request)
HTTP->>API: HTTP request
API-->>HTTP: HTTP response (200/other)
HTTP-->>ApiSetup: response
ApiSetup->>ApiSetup: validate status, unmarshal into target
alt success
ApiSetup-->>Fetch: parsed resource
Fetch-->>Caller: (resource, resource.GetResourceName(), nil)
else failure
ApiSetup-->>Fetch: error
Fetch-->>Caller: (zero, "", error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/speed/speed.go (1)
264-267: Inconsistent error handling:log.Fatalfvs returning errors.Other
strconv.Atoiconversions in this function (lines 276, 281, 285, 290) return errors, but this one callslog.Fatalf, which terminates the program abruptly. This is inconsistent and prevents proper error propagation.speedStageInt, err := strconv.Atoi(pokemon.SpeedStage) if err != nil { - log.Fatalf("Invalid SpeedStage: %v", err) + return "", fmt.Errorf("invalid SpeedStage: %w", err) }
🧹 Nitpick comments (7)
cmd/ability/ability_test.go (1)
4-6: Good new test for hyphenated ability; consider asserting errors as well
- Importing
osand usingos.Argsto simulate CLI input is fine here, and the new"poison-point"case wired toability_poison_point.goldengives nice coverage for special characters in API calls.- As a follow-up, since the table already defines
wantError, you might eventually extend the loop to also assert on theerrorreturned fromAbilityCommand()(especially for the misspelled-ability case), rather than discarding it withoutput, _ := AbilityCommand().No blockers for this PR; just a potential future tightening of the tests.
Also applies to: 45-49, 52-61
card_data/README.md (1)
6-25: Data architecture section is clear; optional wording polishThe new Data Architecture section is informative and reads well. If you want to tighten the wording in step 7, you could adopt the shorter phrasing suggested by the style check:
-7. Users are then able to query the `pokeapi.co` or supabase APIs for either video game or trading card data, respectively. +7. Users can then query the `pokeapi.co` or Supabase APIs for either video game or trading card data, respectively.Purely stylistic; no need to block on this.
cmd/card/imageviewer_test.go (1)
135-153: Consider adding test for error path.There's no test for when
imageReadyMsgcontains an error. Consider adding a test to verify thatUpdatecorrectly setsErrorand clearsImageDatawhen the fetch fails.func TestImageModel_Update_ImageReadyError(t *testing.T) { model := ImageRenderer("Charizard", "http://example.com/charizard.png") expectedErr := fmt.Errorf("failed to fetch image") msg := imageReadyMsg{err: expectedErr} newModel, cmd := model.Update(msg) if cmd != nil { t.Error("Update with error imageReadyMsg should return nil command") } updatedModel := newModel.(ImageModel) if updatedModel.Loading { t.Error("Update with error should set Loading to false") } if updatedModel.Error == nil { t.Error("Update with error should set Error") } if updatedModel.ImageData != "" { t.Error("Update with error should clear ImageData") } }cmd/speed/speed.go (1)
7-7: Remove unused import after fixing error handling.After fixing the
log.Fatalfissue, thelogimport will become unused and should be removed.connections/connection_test.go (3)
235-235: Useassert.Emptyfor consistency.For consistency with the linter's recommendations, use
assert.Emptyhere as well.- assert.Equal(t, "", name, "Expected empty name string on error") + assert.Empty(t, name, "Expected empty name string on error")
288-288: Useassert.Emptyfor consistency.Same recommendation for consistency.
- assert.Equal(t, "", name, "Expected empty name string on error") + assert.Empty(t, name, "Expected empty name string on error")
326-326: Useassert.Emptyfor consistency.Same recommendation for consistency.
- assert.Equal(t, "", name, "Expected empty name string on error") + assert.Empty(t, name, "Expected empty name string on error")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/ci.yml(1 hunks).goreleaser.yml(1 hunks)Dockerfile(1 hunks)README.md(2 hunks)card_data/README.md(1 hunks)card_data/pipelines/poke_cli_dbt/dbt_project.yml(1 hunks)cmd/ability/ability.go(3 hunks)cmd/ability/ability_test.go(2 hunks)cmd/card/imageviewer.go(2 hunks)cmd/card/imageviewer_test.go(2 hunks)cmd/item/item.go(1 hunks)cmd/pokemon/pokemon.go(1 hunks)cmd/speed/speed.go(2 hunks)connections/connection.go(3 hunks)connections/connection_test.go(8 hunks)nfpm.yaml(1 hunks)structs/structs.go(1 hunks)styling/styling.go(1 hunks)testdata/ability_poison_point.golden(1 hunks)testdata/main_latest_flag.golden(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/card/imageviewer.go (3)
cmd/search/search.go (1)
Model(48-56)cmd/card/cardinfo.go (1)
CardImage(23-58)styling/styling.go (2)
Red(14-14)Yellow(16-16)
cmd/ability/ability_test.go (1)
cmd/utils/golden.go (1)
LoadGolden(10-46)
cmd/speed/speed.go (1)
styling/styling.go (1)
Yellow(16-16)
connections/connection.go (1)
styling/styling.go (2)
ErrorBorder(34-36)ErrorColor(33-33)
cmd/ability/ability.go (3)
connections/connection.go (2)
AbilityApiCall(82-84)APIURL(17-17)styling/styling.go (1)
ColoredBullet(17-19)flags/abilityflagset.go (1)
PokemonAbilitiesFlag(31-66)
cmd/pokemon/pokemon.go (1)
connections/connection.go (2)
PokemonSpeciesApiCall(98-100)APIURL(17-17)
🪛 GitHub Actions: Lint
connections/connection_test.go
[error] 121-121: golangci-lint: empty: use assert.Empty (testifylint) instead.
🪛 GitHub Check: linter
connections/connection_test.go
[failure] 121-121:
empty: use assert.Empty (testifylint)
[failure] 159-159:
empty: use assert.Empty (testifylint)
[failure] 197-197:
empty: use assert.Empty (testifylint)
🪛 LanguageTool
card_data/README.md
[style] ~25-~25: As a shorter alternative for ‘able to’, consider using “can then”.
Context: ...a Supabase production schema. 7. Users are then able to query the pokeapi.co or supabase APIs...
(BE_ABLE_TO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (35)
nfpm.yaml (1)
4-4: Version bump in nfpm manifest looks consistent
version: "v1.8.1"aligns with the release bump in the rest of the PR. No other packaging changes here..goreleaser.yml (1)
17-17: GoReleaser ldflags updated to embed v1.8.1The linker flag now sets
main.version=v1.8.1, which matches the new release version used elsewhere.testdata/main_latest_flag.golden (1)
5-5: Confirm--latestgolden vs new binary versionThis golden expects
Latest available releaseto renderv1.8.0, while this PR bumps the tool itself to v1.8.1. If--latestis meant to report the latest GitHub release (not the running binary’s version), this is fine; otherwise, it may need to be updated tov1.8.1.Please confirm the intended semantics of the
--latestflag and adjust the golden if necessary.testdata/ability_poison_point.golden (1)
1-3: New Poison Point golden file is well-formedThe golden output for “Poison Point” matches the existing style (title + bullet details) and provides clear effect text. Looks ready to use in tests.
card_data/pipelines/poke_cli_dbt/dbt_project.yml (1)
2-2: dbt project version and materialization update look good
- Project
version: '1.8.1'matches the overall release bump.+materialized: tableundermodels.poke_cli_dbtis valid dbt config for materializing those models as tables.No further changes needed here.
Also applies to: 21-21
cmd/card/imageviewer.go (7)
3-8: LGTM!The imports are appropriate for the new async image loading feature with spinner support.
10-22: LGTM!The
ImageModelstruct is well-designed with clear separation of concerns between state fields (Loading,Error,ImageData) and configuration (CardName,ImageURL,Spinner). The privateimageReadyMsgtype correctly encapsulates the async fetch result.
24-33: LGTM!The async command correctly wraps the
CardImagecall and properly propagates both success and error states viaimageReadyMsg.
35-40: LGTM!Correct use of
tea.Batchto start both the spinner animation and the image fetch concurrently.
42-65: LGTM!The
Updatemethod correctly handles all message types with proper state transitions. The error/success handling inimageReadyMsgproperly clears the opposite state to maintain consistency.
67-77: LGTM!The
Viewmethod correctly prioritizes states: loading → error → data. The spinner feedback during loading provides good UX.
79-90: LGTM!The
ImageRendererfactory function correctly initializes the model in a loading state with the spinner configured and ready.cmd/card/imageviewer_test.go (8)
10-17: LGTM!Test correctly verifies that
Init()returns a non-nil command, which is expected since it batches the spinner tick and fetch commands.
19-52: LGTM!Tests correctly verify that ESC and Ctrl+C trigger quit commands while maintaining the model type.
54-66: LGTM!Correctly verifies that non-quit keys don't trigger any commands.
68-81: LGTM!Good test coverage for the loading state view, verifying both non-empty output and presence of the card name.
83-97: LGTM!Correctly verifies that when loaded,
View()returns the image data directly.
99-112: LGTM!Good edge case coverage for empty image data scenario.
114-133: LGTM!Comprehensive test for the
ImageRendererfactory function, verifying all essential initial state values.
135-153: LGTM!Excellent test for the async image fetch success path, verifying proper state transitions when
imageReadyMsgis received..github/workflows/ci.yml (1)
33-33: LGTM!Version bump to v1.8.1 is consistent with the PR objectives and will propagate correctly through all workflow jobs.
Dockerfile (1)
11-11: LGTM!Version bump in ldflags is consistent with other version updates in this PR.
cmd/item/item.go (1)
51-55: LGTM!Good refactor to return errors instead of exiting directly. This improves testability and allows callers to handle errors appropriately while still preserving the error message in the output for user display.
cmd/speed/speed.go (1)
308-319: LGTM!The styling updates correctly use the new
Yellow.Render()method, consistent with the refactored styling API.cmd/pokemon/pokemon.go (1)
80-84: LGTM!The change correctly handles the updated
PokemonSpeciesApiCallsignature that now returns an additional resource name string. Discarding it with_is appropriate since the name isn't needed in this context.cmd/ability/ability.go (3)
53-58: LGTM!The error handling improvement correctly returns both the accumulated output and the error, allowing callers to access any partial output before the failure occurred.
60-64: LGTM!Consistent error handling pattern that appends the error to output before returning.
97-101: LGTM!The formatting change to use
fmt.Sprintfis cleaner and more consistent.structs/structs.go (1)
3-25: LGTM!The
GetResourceName()accessor methods correctly implement theEndpointResourceinterface required by the new genericFetchEndpointfunction. Using value receivers for these simple getters is appropriate.styling/styling.go (1)
12-16: LGTM!The refactoring from
YellowAdaptivefunction to aYellowstyle variable is cleaner and more consistent with how other styles are defined in this file. The adaptive color behavior is preserved.connections/connection.go (3)
19-19: LGTM!Using a shared
httpClientwith a 30-second timeout enables connection reuse and prevents resource leaks from creating new clients per request.
65-67: LGTM!Adding the non-200 status code check ensures API errors are properly detected and reported instead of attempting to parse error responses as valid JSON.
82-104: LGTM!The per-endpoint API call functions now cleanly delegate to the generic
FetchEndpoint, eliminating code duplication while maintaining the same public API signatures.connections/connection_test.go (2)
62-87: LGTM!Good addition of comprehensive test coverage for non-200 HTTP status codes. The table-driven test approach is clean and covers multiple failure scenarios.
244-275: LGTM!The expanded
TestTypesApiCallwith proper struct initialization and name verification provides good coverage for the updated API signature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
connections/connection_test.go (1)
244-275: TypesApiCall success test exercises nested payload realisticallyThe
TypesApiCallsuccess subtest builds a representativeTypesJSONStruct(including nestedPokemonentries) and asserts both the struct and returned name, which should catch regressions in JSON wiring and naming behavior. If you later add more type fixtures, consider extracting a small helper to construct these structs to keep the test focused on behavior rather than struct literal noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
connections/connection_test.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
connections/connection_test.go (2)
connections/connection.go (7)
ApiCallSetup(44-80)AbilityApiCall(82-84)ItemApiCall(86-88)MoveApiCall(90-92)PokemonApiCall(94-96)TypesApiCall(102-104)PokemonSpeciesApiCall(98-100)structs/structs.go (6)
AbilityJSONStruct(28-60)ItemJSONStruct(85-107)MoveJSONStruct(110-141)PokemonJSONStruct(144-194)TypesJSONStruct(221-261)PokemonSpeciesJSONStruct(197-218)
🔇 Additional comments (3)
connections/connection_test.go (3)
7-7: Strong coverage for non‑200 ApiCallSetup responsesThe table‑driven subtest across multiple non‑200 status codes plus explicit status‑code matching via
strconv.Itoagives good regression coverage for this path; the addedstrconvimport is appropriate and used correctly.Also applies to: 62-87
117-125: Consistent empty‑name and error semantics across API failure pathsCapturing
namefrom each*ApiCalland asserting the zero‑value struct, emptyname, and the styled, entity‑specific error messages keeps all endpoints’ failure behavior consistent and matches the new FetchEndpoint contract.Also applies to: 155-163, 193-201, 231-239, 284-292, 322-330
308-313: PokemonSpeciesApiCall success path updated correctly for name returnUpdating the Pokémon species test to use the three‑value return and explicitly assert the species name alongside the struct aligns this test with the other endpoint helpers and the new API surface.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/card/imageviewer_test.go (2)
84-91: Strict equality on loaded view is acceptable but may be brittleRight now
View()returning exactlyImageDatamakes this assertion correct. If you later prepend/append metadata (card name, headers, etc.), this test will break; consider loosening to a containment or prefix check if you expect that evolution.
136-153: imageReadyMsg test exercises the full loading→loaded transitionThis neatly covers clearing
Loadingand settingImageData. One minor style tweak you might consider: avoid a potential panic by using the checked assertion pattern:- updatedModel := newModel.(ImageModel) + updatedModel, ok := newModel.(ImageModel) + if !ok { + t.Fatalf("Update should return ImageModel, got %T", newModel) + }Functionally everything is correct as‑is; this just improves failure diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)cmd/card/imageviewer_test.go(2 hunks)styling/styling.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- styling/styling.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/card/imageviewer_test.go (1)
cmd/card/imageviewer.go (2)
ImageRenderer(79-90)ImageModel(10-17)
🔇 Additional comments (8)
README.md (2)
5-5: Version bumps and bracket syntax fix look good.The docker image size badge (line 5) and both docker run examples (lines 98, 102) are now consistently updated to v1.8.1. The bracket syntax has also been corrected from
[subcommand] flag]to[subcommand] [flag], fixing the previous typo flagged in review.Also applies to: 98-98, 102-102
32-32: Verify demo GIF version consistency.Line 32 still references
poke-cli-v1.8.0.gif, while all other docker/version references in the README have been bumped to v1.8.1. Confirm whether a v1.8.1 demo GIF asset exists and should be referenced here, or if the v1.8.0 GIF is intentionally retained.cmd/card/imageviewer_test.go (6)
3-9: Imports look appropriate for new tests
stringsfor substring checks and thespinnerpkgalias forTickMsgkeep the tests readable and focused; nothing to change here.
12-16: Init test correctly guards async startup behaviorAsserting that
Init()returns a non‑nil command (without executing it) is a good, non‑flaky way to ensure the spinner+fetch startup path doesn’t regress to a no‑op.
69-82: Loading view coverage is well targetedChecking that
View()is non‑empty and contains the card name while loading is a solid assertion that avoids brittle coupling to spinner frame output. No changes needed.
100-112: Empty view semantics are clearly definedAsserting that
View()returns an empty string when bothImageURLandImageDataare empty gives a precise contract for the “no image” state; the test looks good.
115-132: ImageRenderer initialization test matches the model contractVerifying
CardName,ImageURL,Loading == true, and emptyImageDatacaptures the key guarantees ofImageRenderer; this should catch most accidental constructor regressions.
156-173: Spinner tick handling test is aligned with the Update logicConstructing a tick message via
msg := model.Spinner.Tick()and passing it throughUpdateis an appropriate way to exercise the spinner‑tick branch and assert that a follow‑up command is returned while preserving theImageModeltype. No issues here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.